Conversation
Micky774
requested review from
ipanfilo,
wangye805 and
wenchenvincent
as code owners
ipanfilo
left a comment
ipanfilo
left a comment
There was a problem hiding this comment.
Maybe make approach more generic - ci_level% label specifies CI level to use.
Micky774
changed the title
| // If a label was added, cancel runs with a lower level than the added label | ||
| if (action === 'labeled') { | ||
| const isLowerThanAdded = runLevel > 0 && runLevel < addedLevel; | ||
| return prMatch && active && isLowerThanAdded && run.id !== currentRunId; |
There was a problem hiding this comment.
can is LowerThanAdded be if run.id === currentRunId?
| .map(label => label.name.toLowerCase()); | ||
|
|
||
| // Determine if a CI level label was removed and what level it was | ||
| const removedLabel = (context.payload.label?.name || '').toLowerCase(); |
There was a problem hiding this comment.
Why is it removedLabel? For labeled event it is addedLabel.
Maybe unlabeled events are not needed at all? If higher lablel is added - CI with new level is triggered. Otherwise the last CI run should still be relevant.
There was a problem hiding this comment.
If a PR has labels ci-level 1 and ci-level 3 but then ci-level 3 is removed, then we still need to run ci-level 1 tests.
There was a problem hiding this comment.
If a PR has labels
ci-level 1andci-level 3but thenci-level 3is removed, then we still need to runci-level 1tests.
It ran Level 3 before, why does it need run Level 1 now? The next time something is changed, yes, but why now?
There was a problem hiding this comment.
Originally it was in case the highest-level test wasn't finished, in which case the commit still wouldn't necessarily have a passed test on the next-highest level yet. I've adjusted the script to explicitly check for tests of at least the new level, and skip dispatch if it exists.
| let requires_dispatch = currentLevel > 0; | ||
| if (action === 'labeled') { | ||
| // Only dispatch when a CI-level label was added. | ||
| requires_dispatch &&= addedLevel > 0; |
There was a problem hiding this comment.
If it had ci-level 2 and then ci-level 1 is added, dispatching is is not needed so rather addedLevel >= currentLevel
| const removedLevel = action === 'unlabeled' ? parseLevelLabel(context.payload.label?.name) : 0; | ||
|
|
||
| // Determine the current highest CI level from remaining labels | ||
| let level = ''; |
There was a problem hiding this comment.
Nit: currentLevel can be assigned right in this block of code, w/o intermediate level
| requires_dispatch &&= addedLevel > 0; | ||
| } else if (action === 'unlabeled') { | ||
| // Only dispatch downgrade when the removed label was the highest, | ||
| // and the now-highest level (if any) does not have a completed run. |
There was a problem hiding this comment.
Why is it needed? If it running level 2 CI and ci-level 2 label is removed, why need to run level 1 CI?
| let test_level = requires_dispatch ? level : ''; | ||
| core.setOutput('test_level', test_level); | ||
| core.setOutput('current_level', String(currentLevel)); | ||
| const shouldCancelOthers = (test_level !== '') || (action === 'unlabeled' && removedWasHighest); |
There was a problem hiding this comment.
Removing label should not automatically cancel running CI
2 participants
Description
With this PR, testing is mediated by labels such as:
ci-level {1, 2, 3}. No tests will be run without at least one label present. At any given time, the highest-level label will take precedence, and only its corresponding level of tests will be run. The workflow triggers on labeling, unlabeling, and pushing commits. When removing a label, the workflow will check to see if there are any labels remaining and if there are, it will dispatch to the highest of them.Fixes # (issue)
Type of change
Changes
Please list the changes introduced in this PR:
Checklist: